Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RHEL-13375: 1.28 Parse URL properly #3345

Merged
merged 8 commits into from
Nov 15, 2023
Merged

RHEL-13375: 1.28 Parse URL properly #3345

merged 8 commits into from
Nov 15, 2023

Conversation

jirihnidek
Copy link
Contributor

@jirihnidek jirihnidek commented Oct 16, 2023 •
edited
Loading

@github-actions
Copy link

github-actions bot commented Oct 16, 2023 •
edited
Loading

Coverage

Coverage (computed on CentOS Stream 9) •
FileStmtsMissCoverMissing
rhsm
   connection.py99247352%48–49, 71, 91–92, 139, 257, 271, 279, 297, 331–336, 340–349, 409, 411, 508, 511, 518–524, 529, 585, 595, 612–613, 622, 625–626, 628–629, 631, 651–653, 657–659, 663–667, 669, 671, 689–696, 698–699, 705, 707, 709–713, 715–726, 728, 731, 733–737, 739–740, 742–743, 754–757, 768–771, 776, 797, 799, 812, 821–825, 827, 832–836, 841–844, 846–851, 855, 857–859, 861–863, 867, 869–870, 879, 881–885, 888–889, 894, 896, 941, 958–961, 988, 1010, 1016, 1019, 1022, 1025, 1049–1050, 1052–1054, 1057, 1093–1097, 1105–1106, 1108, 1116–1117, 1119, 1133, 1143–1145, 1148–1149, 1152, 1165, 1172, 1176, 1190, 1193–1194, 1196–1197, 1199–1200, 1202–1216, 1218–1220, 1222–1233, 1235, 1250–1252, 1254–1256, 1258–1261, 1266–1269, 1293, 1313–1341, 1346–1347, 1349–1351, 1354–1356, 1358–1360, 1363–1364, 1367–1368, 1371–1372, 1394–1395, 1405–1406, 1413–1414, 1420–1422, 1424, 1430–1432, 1434, 1440–1442, 1444, 1450–1451, 1454–1458, 1464–1465, 1471–1472, 1478–1479, 1485–1486, 1504–1507, 1513–1514, 1521–1525, 1531–1532, 1553–1556, 1564–1566, 1593–1594, 1596, 1598, 1601–1602, 1605–1606, 1609–1610, 1613, 1615–1616, 1619, 1630, 1632–1634, 1636, 1638, 1641, 1643–1656, 1658–1659, 1662–1665, 1668–1669, 1672–1674, 1685–1686, 1689–1690, 1692, 1694–1696, 1702–1704, 1713–1715, 1736, 1739–1740, 1743–1744, 1751, 1753–1754, 1756, 1758–1761, 1763–1765, 1768, 1770, 1777, 1779–1780, 1782, 1784–1787, 1789–1791, 1794, 1796, 1799–1800, 1806–1807, 1813–1814, 1820–1823, 1830–1834, 1836–1838, 1844–1846, 1852, 1854–1857, 1859–1860, 1866–1869, 1876–1878, 1884–1886
   utils.py119199%204
subscription_manager
   managercli.py213571766%224, 228, 246, 256–269, 315, 319–320, 333, 355, 359, 361, 401, 439–441, 455–457, 481, 501, 527, 533–534, 536–537, 554, 556–557, 559–561, 563, 638–639, 651, 676, 689–690, 692, 715–716, 725–726, 728, 827, 830–832, 835–838, 846–847, 849–851, 854–856, 859–861, 869–870, 872–874, 880, 903–905, 907–911, 913, 919, 934–937, 939, 948–951, 953, 964, 966, 988–989, 996, 1032, 1034, 1036–1037, 1039, 1049–1050, 1058, 1081–1086, 1096, 1108–1109, 1112, 1188–1192, 1200–1201, 1208, 1211–1212, 1217–1219, 1221–1230, 1232–1235, 1237–1238, 1248–1249, 1251, 1254, 1273–1277, 1301, 1303, 1309, 1312–1314, 1317–1318, 1320–1327, 1329–1332, 1334–1341, 1343, 1345–1346, 1350, 1352–1353, 1355–1358, 1362, 1364, 1366–1367, 1370–1376, 1390, 1392–1393, 1395–1405, 1408, 1410–1415, 1442, 1445, 1451, 1454, 1458, 1465, 1467–1468, 1470, 1472, 1491, 1498–1502, 1504–1511, 1513, 1515–1520, 1523, 1539–1545, 1547–1549, 1591–1592, 1595–1596, 1599–1600, 1602, 1605, 1607–1608, 1610, 1648–1649, 1655–1656, 1658, 1672–1673, 1677–1683, 1686–1692, 1694–1702, 1704–1714, 1725–1727, 1729–1730, 1735, 1740–1746, 1748, 1750, 1753–1754, 1756, 1758–1765, 1767, 1770–1780, 1782, 1836, 1838, 1846, 1851, 1883–1885, 1887–1888, 1895, 1898–1904, 1910–1911, 1913–1914, 1917, 1928, 1932, 1939–1942, 1946–1947, 1951–1952, 1963, 1986, 1990–1992, 2001, 2005, 2026, 2044–2045, 2051, 2083, 2086, 2114, 2118, 2133–2135, 2144, 2170, 2180–2181, 2185–2186, 2188–2192, 2202, 2207, 2213–2214, 2227–2228, 2267–2268, 2274–2275, 2277, 2280–2281, 2283–2284, 2287–2292, 2296–2297, 2299–2301, 2303, 2324–2328, 2335, 2361–2363, 2371, 2379–2383, 2385–2386, 2389–2393, 2460, 2481–2483, 2495–2496, 2499–2500, 2505–2507, 2509–2510, 2512–2514, 2517–2520, 2522–2524, 2526, 2528–2529, 2531–2532, 2534–2545, 2547–2549, 2552, 2554, 2556–2558, 2561, 2563, 2567–2570, 2574–2575, 2579, 2584, 2586–2587, 2589–2590, 2592–2598, 2600, 2602–2603, 2608, 2610, 2612–2613, 2617, 2619, 2674, 2683, 2688, 2692, 2712–2713, 2721, 2737–2743, 2766–2767, 2802–2803, 2806–2807, 2810–2811, 2813–2815, 2817–2821, 2823–2833, 2848, 2851, 2853–2858, 2861–2863, 2865, 2867, 2869, 2872, 2874–2875, 2878–2879, 2883, 2885–2886, 2888, 2890–2892, 2894, 3014–3015, 3065, 3092–3093, 3095, 3168, 3176–3177, 3181, 3236–3237, 3242–3243, 3295, 3299, 3301, 3307, 3337, 3394, 3410–3411, 3419, 3426, 3449–3451, 3455–3456, 3460–3461, 3466, 3501, 3572, 3574, 3576–3578, 3582, 3584, 3586, 3588–3589, 3591–3594, 3596–3597, 3599–3605, 3608, 3610, 3613–3615, 3617–3621, 3624, 3673–3677, 3698–3701, 3732, 3734, 3765–3768, 3779–3780, 3813, 3835–3837, 3839–3844, 3848
   utils.py3397777%63, 66, 93, 150–153, 156–157, 159–161, 164–169, 182, 186, 200–201, 245–252, 256–258, 260–269, 271–272, 275, 287, 289–292, 323–324, 328–330, 334, 356, 362–366, 368–369, 371, 373–375, 377, 411, 419, 429, 626, 628, 658–659
TOTAL22258924558% 

Tests Skipped Failures Errors Time
2368 11 💤 0 ❌ 0 🔥 42.619s ⏱️

@jirihnidek
Copy link
Contributor Author

I believe that this branch can be reviewed as it is and other changes related to IPv6 could introduced in other PRs later, but please do not merge this PR to 1.28 branch ATM.

@ptoscano ptoscano marked this pull request as draft October 17, 2023 06:50
@ptoscano
Copy link
Contributor

Since all the issues that happened after #3302 was merged were identified and fixed, would you please add the backports of the following PRs to this:

Thanks!

@ptoscano
Copy link
Contributor

Also, the issue reference needs to be changed to RHEL-13375.

@jirihnidek jirihnidek marked this pull request as ready for review October 31, 2023 09:57
@jirihnidek jirihnidek changed the title 1878182: 1.28 Parse URL properly RHEL-13375: 1.28 Parse URL properly Oct 31, 2023
@jirihnidek
Copy link
Contributor Author

@pinotree @m-horky I believe that this PR ready for review

ptoscano
ptoscano requested changes Nov 6, 2023
View reviewed changes
Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the backport. It seems mostly OK, there are few differences/changes:

  • in the commit "RHEL-13375: 1.28 Parse URL properly":
    Two test cases were not backported:

    • TestParseUrl.test_ipv4_url
    • TestParseUrl.test_ipv6_url
      One test was moved from this commit to a later one, "Use username and password from --proxy=URL":
    • TestOverrideCommand.test_proxy_user_and_pass_from_url_overridden_by_cli_options

  • in the commit "Improved printing of addresses and URLs"
    There is one change missing:

                      log.debug("Unable to get valid response: %s from CDN: %s" %
                            (result, self.host))

    (self.host was not wrapped using normalized_host())

@jirihnidek
Copy link
Contributor Author

The TestOverrideCommand.test_proxy_user_and_pass_from_url_overridden_by_cli_options was moved to later commit, because it actually wasn't possible to use this test successfully in original commit.

PR is updated.

jirihnidek and others added 8 commits November 15, 2023 11:05
@jirihnidek @ptoscano
RHEL-13375: 1.28 Parse URL properly
415bac1 
* Backport to 1.28 branch
  * Original commit:
    e83e637
* Old tracking number for Bugzilla: 2225403
* Allow parsing of URLs containing IPv6 address like
  http://user:pass@[2001:db8::dead:beef:1]:3128/prefix
* Use properties of urllib.parse to get username,
  password, hostname, port and prefix
* Fixed unit tests for the case "https://www.redhat.com:/en",
  because it is IMHO correct URL
* Extended unit tests related to parse_url().
@jirihnidek @ptoscano
Use parse_url() from utils.py for parsing URL, when --proxy is used.
75f9adb
@jirihnidek @ptoscano
Use username and password from --proxy=URL
f039668 
* When URL in --proxy=URL contains username and password
  (e.g. --proxy=https://user:[email protected]:3128),
  then user and pass are used as proxy_user and proxy_pass
* The --proxyuser and --proxypassword have higher priority.
  Thus, it does not break backward compatibility.
* Added one unit test for this case
@jirihnidek @ptoscano
Improved printing of addresses and URLs
154d985 
* Print IPv6 addresses correctly
* Improved printing of proxy URL in debug mode
@ptoscano
connection: normalize hostname in ConnectionOSErrorException
960a4be 
Use the existing helper to provide a properly formatted hostname for
user displaying (e.g. for exception); this will fix the printing of
IPv6 addresses.

Turn the existing "host" member as property, so the current users keep
working as expected.
@ptoscano
cli: normalize hostname in error message
b58ffa5 
Normalize the hostname printed for connection errors when pinging the
server, in case anything was passed via --serverurl, --insecure, or
--baseurl.
@m-horky @ptoscano
CCT-10: Ensure IPv6-based URLs are properly formatted
c38d6bd 
* Card ID: CCT-10

Recent changes (e83e637) fixed parsing information from IPv6 URLs. This
patch fixes writing these addresses back as strings, mainly into
configuration files.

Previously, passing in IPv6 URL in e.g. `--baseurl` during registration
resulted in broken address in the config file:

$ subscription-manager register --baseurl https://[::1]:8443/prefix
$ cat /etc/rhsm/rhsm.conf | grep baseurl
baseurl=https://::1:8443/prefix

After this patch, the square brackets are written when port is
specified:

$ cat /etc/rhsm/rhsm.conf | grep baseurl
baseurl=https://[::1]:8443/prefix

Due to the state of the code, it is likely that this problem also exists
in other parts. If that is true, it is most likely in less sensitive
parts, such as during logging. Looking back, using `ipaddress` stdlib
would have been the right way to do this, but it is too late to do that.
@jirihnidek @ptoscano
CCT-71: Try to ping server, when --proxy is used
a79a051 
* When --proxy is used, then try to ping server before new
  configuration is used.
ptoscano
ptoscano approved these changes Nov 15, 2023
View reviewed changes
Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@ptoscano ptoscano merged commit 11b7b36 into subscription-manager-1.28 Nov 15, 2023
11 checks passed
@ptoscano ptoscano deleted the jhnidek/1878182 branch November 15, 2023 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ptoscano ptoscano ptoscano approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jirihnidek @ptoscano @m-horky